-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for creating temporary tables in schema #6409
base: 4.3.x
Are you sure you want to change the base?
Add support for creating temporary tables in schema #6409
Conversation
This allows creating temporary tables in PostgreSQL specifying also the on commit options.
`unlogged <https://www.postgresql.org/docs/current/sql-createtable.html#SQL-CREATETABLE-UNLOGGED>`_ | ||
|
||
- **temporary** (boolean): Set a PostgreSQL table type as | ||
`temporary <https://www.postgresql.org/docs/current/sql-createtable.html#SQL-CREATETABLE-TEMPORARY>`_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look like a PostgreSQL-specific concept to me 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it isn't per se but since I have access to PostgreSQL mainly, I implemented it just on the PostgreSQL platform.
What is the status of this PR? I see a failing CI and a feature that works on Postgres only. Are you planning to continue your work? |
I'll try to fix the failing CI later today but I'll keep the implementation to Postgres only. I don't have access to other platforms to try |
In that case, I don't think that we're interested in this change. Either we implement a feature with broad platform support or we don't implement it at all. |
This allows creating temporary tables in PostgreSQL specifying also the on commit options.
…to psql-temporary-table-support
…to psql-temporary-table-support
Can you please rebase your changes? We don't accept PRs with merge commits. |
Digging further in the code, I see that support for temporary tables was implemented but not documented for MySQL. I've implemented the support for all the remaining platforms (except for SQL Server, which doesn't have a different DML to create temporary tables), added tests and documented the feature. |
if (! empty($options['temporary'])) { | ||
$temporary = $options['temporary'] ?? false; | ||
if (! is_bool($temporary)) { | ||
throw new InvalidArgumentException(sprintf( | ||
'invalid temporary specification for table %s', | ||
$name, | ||
)); | ||
} | ||
|
||
if ($temporary === true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a BC break. Do we need this additional strictness?
$statement = match ($options['temporary'] ?? '') { | ||
'' => 'CREATE TABLE ', | ||
'created' => 'CREATE GLOBAL TEMPORARY TABLE ', | ||
'declared' => 'DECLARE GLOBAL TEMPORARY TABLE ', | ||
default => throw new InvalidArgumentException(sprintf( | ||
'invalid temporary specification for table %s', | ||
$name, | ||
)) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so for MySQL we demand that the temporary
property is a bool, but for DB2 we want it to be a string? Can you elaborate on the difference between those two options? Do we need to support both?
|
||
$sqls = parent::_getCreateTableSQL($name, $columns, $options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the call to the parent implementation seems to bloat this method big time. Can we somehow restore that call and make the diff smaller?
@@ -313,9 +317,62 @@ public function getListSequencesSQL(string $database): string | |||
*/ | |||
protected function _getCreateTableSQL(string $name, array $columns, array $options = []): array | |||
{ | |||
$indexes = $options['indexes'] ?? []; | |||
$options['indexes'] = []; | |||
$sql = parent::_getCreateTableSQL($name, $columns, $options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here. Do we have to remove the call to the parent method?
$temporary = match ($options['temporary'] ?? '') { | ||
'' => '', | ||
'global' => 'GLOBAL TEMPORARY ', | ||
'private' => 'PRIVATE TEMPORARY ', | ||
default => throw new InvalidArgumentException(sprintf( | ||
'invalid temporary specification for table %s', | ||
$name, | ||
)) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here. Why are my options for temporary
different for each platform?
TIL: SQL Server implements temporary tables via naming conventions. 🤦🏻♂️ |
Okay, but seriously, what would happen if the app attempted to create a temporary table on SQL server via DBAL? Do we silently create a permanent one? Is that what we want? |
To respond to your comments:
This being the situation, I think there are two alternatives:
To avoid silently creating a permanent table on SQL Server, I would add a check on the SQL Server platform, raising an exception if the temporary option is specified. Last, to avoid overriding the base _getCreateTableSQL method would require a partial rewrite to add at least two extension points, one for the CREATE TABLE modifier and one for the ON COMMIT clause. Plus, all the other platforms already override the method, without calling the parent method at all. |
Thank you very much for your thorough analysis!
This would actually be my favorite. We should be able to define a schema that we can deploy to all supported platforms. And the defaults shouldn't be arbitrary as you put it. We should pick defaults that bring us as close as possible to the behavior supported by the other platforms.
Yeah, that's probably the best we can do. I thought about changing the name according to SQL Server's convention. So, if a temporary table "foo" is to be created, we would create
That sounds about right.
Understood. I wanted to make the life easier for reviewers like myself. But if you say that it's not worth introducing those extensions points, I trust your judgement. |
The difficulty in reducing the schema to a common baseline across all platforms lies in the profound difference between Oracle/DB2 and the rest of the platforms. While on MySQL, PostgreSQL and SQLite temporary tables are only visible to the current session and each session can simultaneously create the same temporary table, on Oracle and DB2 this is not true, temporary table schema is shared among sessions, only data is session-specific. Private temporary tables on Oracle and declared temporary tables on DB2 behave similarly to the other platforms, but only on DB2 we can approximate the behaviour of the other platforms: on Oracle private temporary tables are also required to adhere to a naming convention, bringing up the same problem as on SQL server. It seems to me that keeping the option vendor specific is the better compromise. Perhaps the |
Sorry, my bad, should I rebase on the 4.2.x branch? |
Yes, 4.2.x is the target branch for new features at the moment. Okay, so we have basically two types of temporary tables.
Did I summarize this correctly? Can we fit all implementations into those two types, meaning that all supported DBMS support either type A or type B or both? |
This allows creating temporary tables in PostgreSQL specifying also the on commit options.
Summary
Sometimes it is useful to create temporary tables and this PR adds the support to do so with the schema manager.
I added the handling of the relevant options and I provided tests and documentation.